Support External Postgres DB #276
Conversation
lastFile:ui/src/pages/Settings/MetadataDBFields.tsx
lastFile:ui/src/pages/Settings/MetadataDBFields.tsx
…ol calling enabled
…t variables for H2
…URL and bypassing validation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for external PostgreSQL databases as an alternative to the embedded H2 database for metadata storage. The key feature allows users to configure and validate PostgreSQL connections through the UI while maintaining backward compatibility with H2.
- Introduces metadata database configuration options in the UI with PostgreSQL connection validation
- Adds comprehensive validation for metadata database connections alongside existing storage and model validation
- Updates backend infrastructure to support PostgreSQL metadata databases with proper JDBC handling
Reviewed Changes
Copilot reviewed 37 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/pages/Settings/MetadataDBFields.tsx | New component for configuring metadata database providers with PostgreSQL connection testing |
| ui/src/pages/Settings/AmpSettingsPage.tsx | Integrates metadata database fields and updates validation logic |
| llm-service/app/services/amp_metadata/init.py | Adds PostgreSQL validation logic and metadata database configuration support |
| backend/src/main/java/com/cloudera/cai/util/db/JdbiUtils.java | Adds JDBC connection testing utility with command-line interface |
| backend/src/main/java/com/cloudera/cai/util/db/RdbConfig.java | Updates PostgreSQL URL handling and database name extraction logic |
Comments suppressed due to low confidence (2)
ui/src/main.tsx:54
- [nitpick] The parameter name has changed from
_tofailureCountbut the first parameter in the retry function signature should typically be namedfailureCountor similar. However, the original parameter was intentionally named_to indicate it was unused. Since it's now being used on line 62, the rename is appropriate.
retry: (failureCount: number, error: Error) => {
ui/src/pages/Settings/MetadataDBFields.tsx:115
- [nitpick] The error message could be more specific about which fields are missing. Consider: 'Please provide all required fields: JDBC URL, username, and password.'
messageQueue.error("JDBC URL, username, and password are required for testing connection.");
| str(db_type), | ||
| ] | ||
| try: | ||
| result = subprocess.run(cmd, capture_output=True, text=True, timeout=15) |
There was a problem hiding this comment.
The subprocess call uses user-provided input (db_url, username, password) directly in the command without proper sanitization. While these are passed as separate arguments (safer than shell=True), consider validating input formats to prevent injection attacks.
Copilot uses AI. Check for mistakes.
| @@ -153,7 +153,15 @@ public static String buildDatabaseServerConnectionString(RdbConfig rdb) { | |||
| } | |||
|
|
|||
| if (rdb.isPostgres()) { | |||
There was a problem hiding this comment.
The regex pattern for PostgreSQL URL parsing is complex and could benefit from a comment explaining its structure and what each group captures for future maintainability.
| if (rdb.isPostgres()) { | |
| if (rdb.isPostgres()) { | |
| // Regex pattern to parse PostgreSQL URLs: | |
| // ^jdbc:postgresql:(//[^/]+/)?(\\w+)(.*) | |
| // - (//[^/]+/)?: Matches the optional host and port part of the URL (e.g., //localhost:5432/). | |
| // - (\\w+): Matches the database name (e.g., mydatabase). | |
| // - (.*): Matches any additional parameters or options in the URL (e.g., ?ssl=true). |
Copilot uses AI. Check for mistakes.
…rd in H2 configuration
…g default export for NotFoundComponent.tsx
| if not db_url.startswith("jdbc:"): | ||
| return ValidationResult(valid=False, message="Invalid JDBC URL format.") | ||
|
|
||
| if not username.isalnum() or not (1 <= len(username) <= 16): |
There was a problem hiding this comment.
Why limit the username to 16 characters? Not sure we need this.
There was a problem hiding this comment.
I'm also not sure if alphanumeric usernames are the right restriction
There was a problem hiding this comment.
RDS has these constraints when me and Liu searched it.
No description provided.